Updated hash key generation to handle missing encoding dtype. #167
Conversation
…added tests to validate fix and updated the development change notes.
mx-moth
left a comment
mx-moth
left a comment
There was a problem hiding this comment.
This issue seems to be due to pydata/xarray#2436
| # A float array and an int array mean very different things, | ||
| # but could have identical byte patterns. | ||
| hash_string(hash, data_array.encoding['dtype'].name) | ||
| hash_string(hash, data_array.encoding.get('dtype', data_array.values.dtype).name) |
There was a problem hiding this comment.
The dtype encoding value should be present most of the time. It being missing is an unexpected exception to the norm. Without the context of this bug checking data_array.values.dtype looks unnecessary.
Please add brief one or two line description of why we are doing this extra step, and include a link to the xarray bug report tracking this issue: pydata/xarray#2436
There was a problem hiding this comment.
I've added a comment with issue link and a simple description.
tests/operations/test_cache.py
Outdated
| assert result_cache_key_cf == cache_key_hash_cf1d_sha1 | ||
|
|
||
|
|
||
| def test_cache_key_with_missing_data_array_encoding_type(datasets: pathlib.Path): |
There was a problem hiding this comment.
This test is too specific. What we actually care about is whether datasets opened via xarray.open_mfdataset() produce a valid geometry hash and we still don't have a test for that. Please update this test to ensure that a dataset opened via xarray.open_mfdataset() produces a valid hash. This means the test will continue to pass even if xarray fix the issue with data_array.encoding being empty, and will fail if some other issue with xarray.open_mfdataset() pops up.
There was a problem hiding this comment.
I've added multifile dataset tests. I've included both ugrid and cfgrid specific tests. The cfgrid fixture does lose the encoding as expected, but the test should handle both cases for if and when xarray fixes the issue.
david-sh-csiro
changed the title
david-sh-csiro
deleted the
166-multifile-datasets-dont-work-with-cache-key-generation
branch
Updated hash key generation and added tests to validate the fix.